Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify docs for multiple access controls #24549

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

mosabua
Copy link
Member

@mosabua mosabua commented Dec 20, 2024

Description

Centralize and clarify usage with multiple access control systems.

Additional context and related issues

See #20152

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

@mosabua mosabua requested a review from dain December 20, 2024 18:12
@cla-bot cla-bot bot added the cla-signed label Dec 20, 2024
@github-actions github-actions bot added the docs label Dec 20, 2024
the default `etc/access-control.properties`. Relative paths from the Trino
`INSTALL_PATH` or absolute paths are supported.

The configured access control systems are used in order until access rights are
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this .. also would be good to explain more with an example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is true, but ordering shouldn't matter. Each access control is independent from the others.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well.. if they are independent.. what does that mean .. are the evaluted in order . what if the first on does a deny .. but the second one would be granting access ..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any individual deny results in deny as the final result. So it doesn't really matter which does this first :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if there is a deny in the first access control system .. will it continue evaluating or return immediately without checking any others?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will stop on first deny, but the point is that this should be treated as a set, not a list - so "any of" not "first of". But maybe I'm nit-picking too much :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No .. this is important to know .. so if it is a set .. and not ordered .. does that mean that admins can not rely on the order of the config files and use that for performance improvements .. for example if it is ordered you could recommend that the most restrictive access control is first in the list, or if some are fast (file based maybe) and others are slow (opa with external IO calls and service) then maybe its good to have the fast one first in the list ..

but if the list is not ordered .. all that advice is potentially wrong/meaningless

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the ordered mention for now .. and think we can leave it at that. We can always iterate later as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is ordered, but I would consider this an implementation detail. I don't know if it's intentional, but I would be wary of users actually relying on this ordering.

does that mean that admins can not rely on the order of the config files and use that for performance improvements

Does this actually come up in practice? Access controls are not supposed to be noticeably slow (they run during analysis and making analysis slower makes people nervous, AFAICT). We may be trying to prematurely optimize things.

Also, further in the document we say that configuring multiple access controls is not recommended anyway :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough .. and to be clear .. I am not sure if this comes up in practice .. but I would be surprised if there are not performance hits from it .. esp with more complex ones like OPA .. I know for a fact that the scripts there can be a performance issue.. so the overall access control is then affected.. so it might make sense to be able to give such advice... also I know that in other such setups we talk about order (I think authentication types) and I think it is on purpose there ..

can you make chime in @dain ..

the default `etc/access-control.properties`. Relative paths from the Trino
`INSTALL_PATH` or absolute paths are supported.

The configured access control systems are used in order until access rights are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is true, but ordering shouldn't matter. Each access control is independent from the others.

Copy link
Contributor

@ksobolew ksobolew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mosabua
Copy link
Member Author

mosabua commented Jan 8, 2025

@dain could you look at this so I can merge it ideally.

@mosabua mosabua merged commit 533ac31 into trinodb:master Jan 10, 2025
8 checks passed
@mosabua mosabua deleted the accesscontrol branch January 10, 2025 22:38
@github-actions github-actions bot added this to the 469 milestone Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants